Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Website] - Changes to the Landing Page & Team Page #289

Merged
merged 26 commits into from
May 8, 2023
Merged

Conversation

jasonmosley21
Copy link
Contributor

@jasonmosley21 jasonmosley21 commented May 2, 2023

On the RightOnVideo.jsx we have swapped out the current video with the NBC featured video.
We've also moved the WhyRightOn down within the LandingPage.jsx.
Before:
Screenshot 2023-05-05 at 1 55 10 PM
After:
Screenshot 2023-05-05 at 1 55 32 PM
Then we've added a new Supporters.jsx into the LandingPage.jsx with funder logos.
For all the section titles on the Landing Page we reduced the font size and applied a new classname: landing-page-titles

For the Team page, we added new member profiles by adding photos and 3 new blocks in teamData.js
We also added an advisor tab to the carousel by editing the teamData.js and the rotating carousel function in ProfileAdvisors.jsx

We also add more media queries to the App.scss and landingPage.scss to fix the responsive of the header, header logo and footer logo size on the tablet screen.
Before:
Screenshot 2023-05-05 at 1 48 31 PM Screenshot 2023-05-05 at 1 48 39 PM

After:
Screenshot 2023-05-05 at 1 48 50 PM Screenshot 2023-05-05 at 1 48 54 PM

jasonmosley21 and others added 8 commits May 1, 2023 12:54
adding NBC Video to home page as well as sponser
updated the 4.0 logo and changed the section heading font sizes
update on team member and advisor
photos
updated section headings to one singular component as well as fixing the team page to actually display and show Yong Lin's photo
@jasonmosley21 jasonmosley21 marked this pull request as ready for review May 2, 2023 20:01
@cindyqquach cindyqquach requested a review from drewjhart May 2, 2023 21:36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nitpicky but can we rename this file so that it doesn't include a period in the filename itself? 4point.svg or something?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point -- 4.0 oftentimes uses 4pt0, so we might go with that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, nice one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, if we are using hyphens between words (like with andy-circle.png) we should probably be consistent throughout the folder. So this would be coming schmidt-futures.svg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just renamed it with the rest of the logos!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you both need to make these changes to get the environment running? It's probably not a big deal but can you provide some context to explain why each of the versions are changing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually don't know what happened but currently we both have the node-sass: 8.0 version on our local folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, ok sounds good. let's go with these versions then.

height: 100%;
z-index: 3;
top: 0;
left: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: indent to match line 239

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks way better, thanks

width: 100%;
height: 100%;
position: relative;
padding-bottom: 56.25%; /* 16:9 aspect ratio */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind giving me a bit more info on the padding-bottom property here? I'm wondering how the padding itself impacts the layout and how you arrived at 56.25%

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're using SASS, so this value can be extracted into a variable and used across the file instead of duplicating the value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can explain the padding-bottom value. When I switched the video it would get these white borders on the sides, I'm assuming because of the aspect ratio not matching the container, so I ask ChatGPT and it give my this bit of code to fix the issue and that value is based on the aspect ratio of the video. 9/16

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, learned something new every day. Apparently, iframes don't work like images in terms of scaling. Maybe it's just me but I think including this link: http://alistapart.com/article/creating-intrinsic-ratios-for-video/ would help as a comment (instead of 16:9 aspect ratio) so that other people who see this know where the padding-bottom is coming from.

And Mani's point is a good one. you can define a variable in the scss file like: $video-aspect-ratio: 56.25%; (underneath the imports in the file) and then use it like padding-bottom: $video-aspect-ratio (in .right-on-video-container). See this for reference: https://sass-lang.com/documentation/variables and shoot me a message on slack if you need a hand

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha I did add the variable and placed the link in the file since it does a better job of explaining how that works!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2023-05-05 at 1 45 50 PM

Screenshot 2023-05-05 at 1 54 59 PM

Also Drew here's some screenshots of what problems i was getting with the old code and why I used the padding-bottom tag

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally, I did some testing and got the exact same result but didn't know about the padding-bottom trick. Good to know! I think adding the url will help clarify this if it isn't common knowledge. nice job

Copy link
Collaborator

@maniramezan maniramezan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on the first PR, this is amazing, especially the new components you've created. I left some minor notes and also:

  • Checkout markdown on Github for learning better about rich text editing for PR notes and generally anywhere in Github
  • It's always nice to include a screenshot of the work when your changes is adding a new UI component or changing the existing behavior. You can even attach a video to your PRs

@@ -33,6 +33,9 @@
letter-spacing: 0em;
text-align: center;
color: white;
justify-items: center;
display: grid;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line, you can also remove the other extra line that was here as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cindyqquach @jasonmosley21 do you mind double checking that you've resolved Mani's formatting comments? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think we got all of them!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@@ -41,6 +44,8 @@
border: 3.5px solid #E29B5D;
opacity: 1;
justify-content: center;
align-items: center;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line, usually there shouldn't be an extra line before }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

width: 100%;
height: 100%;
position: relative;
padding-bottom: 56.25%; /* 16:9 aspect ratio */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're using SASS, so this value can be extracted into a variable and used across the file instead of duplicating the value

@@ -103,6 +128,12 @@
margin-left: 15px;
}

.supporters{
display: flex;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line here and generally all over this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the file and tried to remove any extra lines!

</Grid>
<RightOnVideo/>
<Grid item sm={12} style={{ zIndex:2 }}>
<SectionHeading title="Why RightOn"style={{ zIndex:2 }}/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a question mark in the title?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was supposed to me an exclamation mark which has been fixed!

Comment on lines 17 to 25
<Grid item style={{ height: '130px' }}>
<div style={{height: '130px'}} className='supporters'>
<img
src={featureImage}
alt="right-on-education-product-features"
style={{ marginLeft: "60px", marginRight: "60px" }}
justifyContent="center" alignItems="center"

/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

{supporters.map((logos, key) => {
return (
<Grid item>
<Logos key={key} logos={logos} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something here, but what's this component for? Is this for showing the logo for each supporter? Can one support have multiple logos? Or is it just one logo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each supporter has just one logo. I used this component to handle organizing the logos.

Copy link
Contributor

@drewjhart drewjhart May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think we can probably simplify this. I think in terms of layout we have a title ("With Support From") and then a grid containing the logos of the sponsors. We are using both Box and Grid mui components as containers.

The mui Grid component is nice because it handles reorienting multiple components in a grid layout (particularly across different sized devices). So I think we only want to use it when we have multiple items in our container. We also can get all the functionality we need out of it with just one Grid container and a series of Grid items. In other cases we can just use Box as a generic mui container.

If we follow this logic, we can actually just hoist the img from Logos and put it into this component, removing the other grid items in that component. So I think we would end up with this:

Suggested change
<Logos key={key} logos={logos} />
function Supporters(props) {
const { supporters } = props
return (
<Box component="section">
<Box justifyContent="center" alignItems="center" >
<SectionHeading title="With support from" />
</Box>
<Grid container justifyContent="center" alignItems="center" spacing={5} >
{supporters.map((logo) => (
<Grid item>
<img
src={logo.featureImage}
alt="right-on-education-product-features"
style={{
height: '130px',
width: '350px',
marginLeft: "60px",
marginRight: "60px"
}}
/>
</Grid>
))}
</Grid>
</Box>
);
}

(note that the above suggestion replaces everything from lines 11 - 40 in the original Supporters.jsx and allows Logos.jsx to be deleted.

@cindyqquach maybe give this a shot and see what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I update the code and it works fine, I just had to adjust the height and width a little. Should be good now!

Copy link
Contributor

@drewjhart drewjhart May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @jasonmosley21, got confused on who was working on what here. Thanks for addressing this.

Comment on lines 21 to 35
<Grid
item
container
spacing={5}
justify="center"
className="why-cards-section"
>
{supporters.map((logos, key) => {
return (
<Grid item>
<Logos key={key} logos={logos} />
</Grid>
);
})}
</Grid>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 38 to 42
src={topBackground}
className='why-section-top-background'
// width="100%"
alt="math-background"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

cindyqquach and others added 10 commits May 3, 2023 16:07
updated the NSF logo, and fix issues with the video component responsiveness. background texture is a work in progress
added width to the logos to center them and also adding some margin to the grid to increase spacing from the footer
for the table view I made the line height smaller. and added spacing between the text and underline.
@drewjhart
Copy link
Contributor

Nice job on the first PR, this is amazing, especially the new components you've created. I left some minor notes and also:

  • Checkout markdown on Github for learning better about rich text editing for PR notes and generally anywhere in Github
  • It's always nice to include a screenshot of the work when your changes is adding a new UI component or changing the existing behavior. You can even attach a video to your PRs

@cindyqquach @jasonmosley21 Appreciate that you both added some descriptions to the PR - that helps a lot in understanding the changes. Would you also be able to add some screenshots to the description per Mani's comment above? Feel like it makes it a ton easier to understand what the PR is about.

jasonmosley21 and others added 6 commits May 5, 2023 11:02
updates the frame around the logos to have less space, updated the right-on-video-container comments to help other understand the values, and updating indentation and removing spaces
updated the code to be a little more simplified. with that i had to adjust the logos again to be more aligned.
the section heading was behind the background
@cindyqquach cindyqquach merged commit 9268276 into main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants